Skip to content

feat(vault): implement key versioning and rotation for encrypted secrets (#203)#310

Open
vansh-09 wants to merge 19 commits into
utksh1:mainfrom
vansh-09:feat/cred-lifecycle
Open

feat(vault): implement key versioning and rotation for encrypted secrets (#203)#310
vansh-09 wants to merge 19 commits into
utksh1:mainfrom
vansh-09:feat/cred-lifecycle

Conversation

@vansh-09
Copy link
Copy Markdown

@vansh-09 vansh-09 commented May 25, 2026

Description

Implements vault key versioning and a transactional key rotation workflow:

  • Add key_version column to credential_vault rows and DB migration support.
  • Implement VaultCrypto version-aware encrypt/decrypt helpers and the ability to try multiple keys when decrypting.
  • Add POST /api/v1/vault/rotate endpoint that accepts the previous key (old_key) and attempts to re-encrypt all stored vault entries with the current key in a single transaction. If any entry cannot be decrypted, the rotation aborts and the DB is rolled back.
  • Ensure reads continue to succeed for legacy entries when the server knows the previous key: either via SECUSCAN_VAULT_KEY_PREVIOUS environment variable or because old_key was supplied to the rotation endpoint (the endpoint temporarily records it in memory).
  • Add unit tests covering successful rotation, missing-key validation, and rollback-on-failure scenarios.
  • Add documentation docs/vault-rotation.md describing operator workflow, API usage, and security notes.

Related Issues

Fixes #203

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Automated unit tests added: testing/backend/unit/test_vault_rotation.py (covers rotate success, missing key, and rollback behavior).
  • Run the vault tests locally:
# from repo root, with the project's virtualenv activated
.venv/bin/python scripts/run_vault_tests.py
# or use pytest directly
pytest -q testing/backend/unit/test_vault*.py
  • Example manual reproduction:
  1. Write a secret using the current key:
curl -X PUT "http://127.0.0.1:8000/api/v1/vault/my-secret" -d 's3cr3t'
  1. Rotate by supplying the previous seed:
curl -X POST -H "Content-Type: application/json" \
  -d '{"old_key":"previous-seed"}' \
  http://127.0.0.1:8000/api/v1/vault/rotate
  1. Confirm secrets are readable after a successful rotation with GET /api/v1/vault/{name}.

Notes: In CI or production, prefer configuring SECUSCAN_VAULT_KEY_PREVIOUS in the process environment before rotation so the server can decrypt legacy entries across restarts.

Test results (local): 16 passed, 1 warning.

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

@utksh1 utksh1 added area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests type:security Security work category bonus label type:feature Feature work category bonus label level:critical 80 pts difficulty label for critical or high-impact PRs labels May 26, 2026
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. This overlaps with the vault crypto fix in #279 and should be redesigned on top of AES-GCM. Please avoid accepting old vault keys in an API request body, avoid mutating global settings during a request, and add a migration/rotation flow that fails closed on mixed or undecryptable records.

@vansh-09 vansh-09 force-pushed the feat/cred-lifecycle branch 3 times, most recently from 0f319e1 to ae48084 Compare May 26, 2026 14:59
@vansh-09 vansh-09 force-pushed the feat/cred-lifecycle branch from 874bd71 to 4c8ea0b Compare May 26, 2026 15:24
@vansh-09
Copy link
Copy Markdown
Author

@utksh1 please review this PR.
also this one test is unresponsive and is in progress state for a while. tried to tackle it but the results are same.

@utksh1
Copy link
Copy Markdown
Owner

utksh1 commented May 28, 2026

Thanks for following up. Clarifying the change request so it is actionable:

Why this is blocked:
Requesting changes. This overlaps with the vault crypto fix in #279 and should be redesigned on top of AES-GCM. Please avoid accepting old vault keys in an API request body, avoid mutating global settings during a request, and add a migration/rotation flow that fails closed on mixed or undecryptable records.

What to do next:

  • Fix the specific issues called out above.
  • Push the updated branch and make sure the relevant CI checks pass.
  • Reply here when ready for re-review.

Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the update, but backend-tests are still failing and the PR changes CI to skip benchmark/frontend unit/build on pull_request, which is unrelated and weakens verification. Please restore CI behavior, fix backend tests, and keep the vault rotation change focused.

@utksh1
Copy link
Copy Markdown
Owner

utksh1 commented May 29, 2026

Current status: still blocked. The credential lifecycle/key-rotation PR is conflicting with current main. Please rebase and keep migration/backward-compatibility and secret redaction behavior explicit, with focused vault tests.

@utksh1
Copy link
Copy Markdown
Owner

utksh1 commented May 30, 2026

Re-reviewed after the latest push. Still blocked: backend tests are not complete/green yet and the PR still changes CI behavior outside the vault-rotation scope. Please restore unrelated CI behavior, fix backend tests, and keep migration/backward-compatibility plus redaction coverage focused on vault rotation.

@utksh1
Copy link
Copy Markdown
Owner

utksh1 commented May 31, 2026

Re-reviewed after the latest push. Still blocked: vault rotation needs green backend tests and should not change unrelated CI behavior. Please keep the PR focused on key versioning/migration/backward-compatibility and secret redaction coverage.

Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. This still cannot be merged because the current head is behind main and the required backend-tests check is cancelled. For a vault key rotation/security PR, I need a clean full CI run on the current base before approval.

Please update the branch with latest main and rerun CI so backend-tests completes successfully. I will re-review the implementation after the branch is current and the required tests are green.

Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still cannot merge.

The current head changes .github/workflows/ci.yml so several checks only run on push (benchmark, frontend unit tests, frontend build). On the PR itself, backend-tests is cancelled and benchmark is skipped, which means this critical vault/security change is not being validated by the full required suite. Please remove the PR-check bypasses, update the branch with latest main, and rerun CI until backend-tests completes successfully.

Also, the new POST /vault/rotate endpoint should be reviewed as an operator/security action: it currently sits behind the normal API router but has no explicit admin dependency. If rotation is intended to be admin-only, add the admin authorization dependency and route tests for non-admin rejection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:critical 80 pts difficulty label for critical or high-impact PRs type:feature Feature work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SECURITY] Add vault key rotation with backward-compatible credential migration

2 participants